Repository navigation
fix(registry): centre mk-callout-highlight with flex instead of a script-time measurement - #5118
jbernard077 wants to merge 2 commits into
Conversation
…ipt-time measurement The text box becomes a flex item of a full-frame wrapper (#mk-ch-center), so its position is laid out by the browser rather than read from a getBoundingClientRect() taken at script time. That measurement depends on which font faces are loaded when the script runs; the fonts gate covers only faces that static markup already uses, so script-built text measures against the fallback face (heygen-com#5112). A non-null CONFIG.x / CONFIG.y becomes a margin offset on that axis. The GSAP y entrance stays on the box, which carries no CSS transform. Also drops the scene-exit visibility tail (the runtime hides the clip at its window end; the 0.02 s tail never reached a frame at 30 fps) and removes data-layout-allow-occlusion from #mk-ch-box: nothing beneath the block is text, and the attribute made the contrast audit skip the block's own words whenever a later full-frame host answered the hit-test probe. Renders are bit-identical to the measured version when the face is loaded (nine-scene project at v0.8.135, 1380/1380 frames). CONFIG and the sweep mechanic are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Change 1 is good. Please split out changes 2 and 3. Also note that this PR works around #5112 in one block. It doesn't fix #5112, which is about the runtime's fonts gate, so #5112 should stay open after this merges.
- Flex centring (
mk-callout-highlight.html:33-45,:65-67,:124-136): approve. It removes the script-timegetBoundingClientRect()that depends on font timing. With the defaultx: 1030the flex-start plusmarginLeftpath puts the left edge at the same place as before, the free axis stays centred, andflex: nonewithmaxWidthgives the same width as the old shrink-to-fit box as long asx + maxWidthfits in the frame. One small difference: whenx + maxWidth > 1920, the old absolute box shrank to the space left and wrapped earlier, whileflex: nonedoesn't shrink and gets clipped by the root'soverflow: hidden. That's an edge case, but worth a line in the CONFIG comment. - Removing the
DUR - 0.02visibility tail is a behavior change, and the claim behind it holds only at 30 fps. At 60 fps, frame 479 is at 7.983 s, after the 7.98 s tail, so that last frame used to be hidden. WithanimationOut: false, which is a published CONFIG option, the last frame now shows the text. If a host gives the block a window longer than 8 s, the text now stays up past the block's own timeline. This tail is also a family-wide convention, used in more than ten other registry blocks (mk-specs-list, mk-progress-stat, mk-line-graph, hw-*, ...). Removing it from one block makes this block the odd one out. If the tail should go, do it in its own PR across the family. - Removing
data-layout-allow-occlusion: the reason given is the contrast-audit probe bug, which #5202 has since fixed on main ("audit text beneath transparent composition boxes"). With that fixed, this change doesn't need to be in this PR. It's also one of nine blocks with that attribute. Please drop it here, or give a reason that still applies after #5202.
Not run locally. I'm relying on the bit-identical render comparison in the PR body and green CI for change 1.
— Rames
Address review: restore the scene-exit visibility tail and the layout occlusion attribute, which belong to family-wide conventions and are out of scope here, and note the x + maxWidth clip edge in CONFIG. Refs heygen-com#5112 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the review. Done in the follow-up commit:
PR body updated to one change; it works around #5112 in this block and does not fix it, so #5112 stays open. One thing worth knowing since you are relying on the render comparison: the bit-identical result holds only when the face is already loaded when the script runs (that nine-scene project has it in static markup). Rendering the block on its own, main and this branch differ on 227 of 240 frames, and that difference is the defect: an instrumented copy shows both versions measure the box at two lines (156 px, fallback face, fonts still loading) at script time and both end at three lines (234 px, the real face) once fonts are ready. Main keeps the stale top and its box sits 39 px below centre; this branch is re-centred by layout. Main rendered twice is bit-identical, so this is not noise. Details are in the updated Verification section. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 81b295d, which clears my earlier request for changes. All three points from that review are addressed:
- Flex centring: kept, and unchanged from the version I approved (
mk-callout-highlight.html:33-46,:65-67,:126-139). Thex + maxWidth > 1920clipping edge case now has a note in the CONFIG comment (:91-93), as I asked. - The
DUR - 0.02visibility tail is back (tl.set(root, { visibility: "hidden" }, DUR - 0.02)), so the block matches the rest of the family again. data-layout-allow-occlusionis back on#mk-ch-box.
I diffed the head against its base and against current main. The only change left is the flex centring, its comments and the CONFIG note, and it merges cleanly with main. No later commit on main touches this block.
One thing to fix before merge, and it's not about the code: GitHub lists #5112 as an issue this PR will close, probably through the Development sidebar link. The description rightly says this works around #5112 and doesn't fix it. Please unlink #5112 so it stays open when this merges.
Not run locally. For the render, I'm relying on the frame comparison in the description, and CI is green at this head.
— Rames
This branch has not been deployed
2 participants
Summary
One change to
registry/blocks/mk-callout-highlight/mk-callout-highlight.html: flex centring replaces the script-time measurement. The text box is a flex item of a full-frame wrapper (#mk-ch-center), so the browser lays out its position instead of the script readinggetBoundingClientRect()once at load. The measured version depends on which font faces are loaded when the script runs: the runtime's fonts gate covers only faces that static markup already uses, so a box whose words are built by script measures against the fallback face. This works around #5112 for this block. The underlying defect, the runtime's fonts gate, is tracked in #5112 and stays open after this merges.A non-null
CONFIG.x/CONFIG.yswitches that axis to an edge offset via a margin. Withflex: nonethe box keeps its content width (clamped bymaxWidth) and no longer shrinks whenx + maxWidthexceeds the frame; the CONFIG comment says to keepx + maxWidth <= 1920. The GSAP entrance / exitytween stays on the box, which carries no CSS transform. Nopointer-events: noneon the wrapper, on purpose: it would inherit to the word spans and the contrast audit would skip them. TheCONFIGsurface, the word-sweep mechanic, the scene-exit tail and the layout attributes are unchanged.Verification
index.html, same CLI,--workers 1): main and this branch differ on 227 of 240 frames, by a 39 px vertical offset of the whole box, identical wrap and x. That is the defect, not noise: an instrumented copy shows both versions measure the box at 156 px (two lines, fallback face,document.fonts.statusstillloading) when the script runs, and both end at 234 px (three lines, the real face) once fonts are ready. Main keeps the top computed from the stale 156 px measurement, so its box sits 39 px below centre; this branch is re-centred by layout. Main rendered twice is bit-identical, so the difference is not render noise.hyperframes lintandhyperframes checkon the block: 0 errors, 0 warnings across lint, runtime, layout, motion and contrast.node scripts/lint-registry-items.mjs mk-callout-highlight: passed.bun run check:catalog-drift: catalog sources generate successfully. Generated files are not committed, per CONTRIBUTING.🤖 Generated with Claude Code